Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

diagnostics: if AssocFn has self argument, describe as method #108324

Merged
merged 4 commits into from
Feb 23, 2023
Merged

diagnostics: if AssocFn has self argument, describe as method #108324

merged 4 commits into from
Feb 23, 2023

Conversation

notriddle
Copy link
Contributor

@notriddle notriddle commented Feb 21, 2023 •
edited
Loading

Discussed in https://rust-lang.zulipchat.com/#narrow/stream/147480-t-compiler.2Fwg-diagnostics/topic/.22associated.20function.22.20vs.20.22method.22/near/329265515

This commit also changes the tooltips on rustdoc intra-doc links targeting methods.

For anyone not sure why this is being done, see the Reference definitions of these terms in https://doc.rust-lang.org/1.67.1/reference/items/associated-items.html#methods

Associated functions whose first parameter is named self are called methods and may be invoked using the method call operator, for example, x.foo(), as well as the usual function call notation.

In particular, while this means it's technically correct for rustc to refer to a method as an associated function (and there are a few cases where it'll still do so), rustc must never use the term "method" to refer to an associated function that does not have a self parameter.

@rustbot
Copy link
Collaborator

rustbot commented Feb 21, 2023

r? @davidtwco

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Feb 21, 2023
@rustbot
Copy link
Collaborator

rustbot commented Feb 21, 2023

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

Some changes occurred in need_type_info.rs

cc @lcnr

@rust-log-analyzer

This comment has been minimized.

Sign in to view
compiler-errors
compiler-errors reviewed Feb 22, 2023
View reviewed changes
Copy link
Member

@compiler-errors compiler-errors left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a fan of this change, but maybe cc @rust-lang/wg-diagnostics on this, since it kinda solidifies our usage of some terminology that has been up in the air in the past.

r=me on the impl modulo one nit

src/tools/clippy/clippy_lints/src/missing_doc.rs Outdated Show resolved Hide resolved
@rustbot
Copy link
Collaborator

rustbot commented Feb 22, 2023

Some changes occurred in src/tools/cargo

cc @ehuss

@compiler-errors
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Feb 22, 2023

📌 Commit bf9b980 has been approved by compiler-errors

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 22, 2023
@compiler-errors
Copy link
Member

@bors p=1

given the number of files this touches, this may be bitrotty.

@compiler-errors
Copy link
Member

@bors r- r=compiler-errors,davidtwco,estebank,oli-obk

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Feb 22, 2023
@bors
Copy link
Contributor

bors commented Feb 22, 2023

📌 Commit bf9b980 has been approved by compiler-errors,davidtwco,estebank,oli-obk

It is now in the queue for this repository.

@bors bors removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Feb 22, 2023
@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Feb 22, 2023
@bors
Copy link
Contributor

bors commented Feb 23, 2023

⌛ Testing commit bf9b980 with merge 0978711...

@bors
Copy link
Contributor

bors commented Feb 23, 2023

☀️ Test successful - checks-actions
Approved by: compiler-errors,davidtwco,estebank,oli-obk
Pushing 0978711 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Feb 23, 2023
@bors bors merged commit 0978711 into rust-lang:master Feb 23, 2023
@rustbot rustbot added this to the 1.69.0 milestone Feb 23, 2023
@bors bors mentioned this pull request Feb 23, 2023
Merged
@notriddle notriddle deleted the notriddle/assoc-fn-method branch February 23, 2023 03:56
@xFrednet xFrednet mentioned this pull request Feb 23, 2023
Closed
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (0978711): comparison URL.

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)		 1.1% [1.0%, 1.2%] 2
Regressions ❌
(secondary)		 2.2% [2.0%, 2.6%] 6
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 -2.3% [-2.3%, -2.3%] 1
All ❌✅ (primary) 1.1% [1.0%, 1.2%] 2

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)		 - - 0
Regressions ❌
(secondary)		 2.5% [2.5%, 2.5%] 1
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) - - 0

Cycles

This benchmark run did not return any relevant results for this metric.

@rustbot rustbot added the perf-regression Performance regression. label Feb 23, 2023
@nnethercote
Copy link
Contributor

Noise.

@rustbot label: +perf-regression-triaged

@rustbot rustbot added the perf-regression-triaged The performance regression has been triaged. label Feb 24, 2023
Jarcho pushed a commit to Jarcho/rust that referenced this pull request Feb 26, 2023
@bors
Auto merge of rust-lang#108324 - notriddle:notriddle/assoc-fn-method,…
daa94d2 
… r=compiler-errors,davidtwco,estebank,oli-obk

diagnostics: if AssocFn has self argument, describe as method

Discussed in https://rust-lang.zulipchat.com/#narrow/stream/147480-t-compiler.2Fwg-diagnostics/topic/.22associated.20function.22.20vs.20.22method.22/near/329265515

This commit also changes the tooltips on rustdoc intra-doc links targeting methods.

For anyone not sure why this is being done, see the Reference definitions of these terms in <https://doc.rust-lang.org/1.67.1/reference/items/associated-items.html#methods>

> Associated functions whose first parameter is named `self` are called methods and may be invoked using the [method call operator](https://doc.rust-lang.org/1.67.1/reference/expressions/method-call-expr.html), for example, `x.foo()`, as well as the usual function call notation.

In particular, while this means it's technically correct for rustc to refer to a method as an associated function (and there are a few cases where it'll still do so), rustc *must never* use the term "method" to refer to an associated function that does not have a `self` parameter.
@clubby789 clubby789 mentioned this pull request Feb 27, 2023
Closed
@notriddle notriddle mentioned this pull request Feb 27, 2023
Merged
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 27, 2023
@matthiaskrgr
Rollup merge of rust-lang#108533 - notriddle:notriddle/resolver-def-d…
a184150 
…escr, r=compiler-errors

diagnostics: avoid querying `associated_item` in the resolver

Fixes rust-lang#108529

CC rust-lang#108324
@Y-Nak Y-Nak mentioned this pull request Apr 22, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@compiler-errors compiler-errors compiler-errors left review comments

@estebank estebank estebank approved these changes

@davidtwco davidtwco davidtwco approved these changes

@oli-obk oli-obk oli-obk approved these changes

Assignees

@davidtwco davidtwco

Labels
merged-by-bors This PR was explicitly merged by bors. perf-regression Performance regression. perf-regression-triaged The performance regression has been triaged. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.69.0
Development

Successfully merging this pull request may close these issues.
10 participants
@notriddle @rustbot @rust-log-analyzer @compiler-errors @bors @rust-timer @nnethercote @oli-obk @davidtwco @estebank